chore: switch from travis to github actions for ci #989
Conversation
| @@ -19,7 +19,7 @@ const playFor = function(player, time, cb) { | |||
|
|
|||
| const checkPlayerTime = function() { | |||
| window.setTimeout(() => { | |||
| if (player.currentTime() <= targetTime) { | |||
| if (player.tech_ && player.tech_.el_ && player.currentTime() <= targetTime) { | |||
There was a problem hiding this comment.
This prevents an error from being logged when this functoin runs after the player has disposed on a test timeout.
brandonocasey
changed the title
.github/workflows/npm-test.yml
Outdated
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| os: [macos-latest] |
There was a problem hiding this comment.
I think that in the future it will be possible for us to test on windows and drop browserstack entirely. Not that I hate browserstack but it would allow us to unify testing in pull requests and pushes. Just have to drop ie 11 support...
.github/workflows/ci.yml
Outdated
| # run safari, chrome, firefox on macos | ||
| - name: Run Mac test | ||
| run: npm run test | ||
| if: ${{ startsWith(matrix.os, 'macos') }} | ||
|
|
||
| # only run ie 11 on windows | ||
| - name: Run Windows test | ||
| run: npm run test | ||
| if: ${{ startsWith(matrix.os, 'windows') }} | ||
|
|
||
| # run chrome/firefox in linux |
There was a problem hiding this comment.
is it possible to run these in parallel?
Though, we can do that as a followup too.
There was a problem hiding this comment.
Though, currently we're only running on linux for BS?
I'm OK with making these updates as a follow-up.
There was a problem hiding this comment.
all os steps run in parallel yeah, we are only running bs on linux
There was a problem hiding this comment.
We are actually only running on linux right now period for everything, as windows/mac are not included in the os matrix list at all.
There was a problem hiding this comment.
cool, yeah, wasn't sure whether the matrix items run in parallel or not.
Taken from videojs/http-streaming#989
| @@ -1 +1 @@ | |||
| lts/dubnium | |||
| 10 | |||
There was a problem hiding this comment.
This was switched to a number because actions/setup-node requires a number and can't use lts/dubnium. While I still like the signal that we use lts here, having a single source of truth for the node version we're on is nice.
More updates from videojs/http-streaming#989. Makes it actually ready for usage now. As the description says, it requires videojs-generate-karma-config@7 and newer, karma@5, and a number in the nvmrc file. We read the nvmrc to know the version to use in the CI action and it can only use a number directly. Having a single source of truth is better than having a signal that we're on an lts version.
This switches our code to be tested on github actions as travis ci has become unreasonably slow.
Depends upon videojs/videojs-generate-karma-config#37
The tests for this pull request are run in a forked branch to show that they can work for a fork. To see local tests using browserstack see https://github.com/videojs/http-streaming/runs/1360756885?check_suite_focus=true